os: add availableParallelism() #45895
Conversation
nodejs-github-bot
added
c++
cjihrig
force-pushed
the
avp
branch
2 times, most recently
from
16c93ee
to
9a3b8a8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
cjihrig
added
the
author ready
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| * Returns: {integer} | ||
|
|
||
| Returns an estimate of the default amount of parallelism a program should use. | ||
| Always returns a value greater than zero. |
There was a problem hiding this comment.
Some explanation about how that estimate is reached would be helpful here.
There was a problem hiding this comment.
Here is the PR that added this functionality to libuv - libuv/libuv@f250c6c. If you have some exact text you'd like to see based on that, I'm happy to add it. The CI has been so flaky (and frustrating) that I don't want to go back and forth with pushing changes, running the CI, making more changes, etc. - I'd rather just ship whatever suggestions you have.
Alternatively, a docs update could be added after the fact since the CI requirements for docs only changes are much lower.
There was a problem hiding this comment.
The implementation of uv_available_parallelism() will likely evolve over time so the docs here shouldn't be too specific, or they'll become outdated.
My suggestions are to a) link to the libuv documentation, b) leave it out, or c) say we consult the animating spirits inside the computer.
There was a problem hiding this comment.
I'd like to see how option C plays out.
There was a problem hiding this comment.
Returns an estimate of the default amount of parallelism a program should use. Always returns a value greater than zero.
On Linux, inspects the calling thread’s CPU affinity mask to determine if it has been pinned to specific CPUs.
On Windows, the available parallelism may be underreported on systems with more than 64 logical CPUs.
On other platforms, reports the number of CPUs that the operating system considers to be online.
The heuristics used are expected to evolve over time and may vary across operating systems.
Or, if nothing else, link to the libuv documentation.
There was a problem hiding this comment.
Does not answer the question of what it does inside of a cgroup, which is the primary problem with cpus()
There was a problem hiding this comment.
It's not mentioned because it's oblivious to the presence of cgroups.
There was a problem hiding this comment.
I'm not a fan of adding new features that replicate open bugs in existing functionality.
Also #28855 specifically asks for this problem (cgoups causing process count to be wrong) to be fixed, which is what my question is asking. Obliviousness is exactly the problem. cpus().length is oblivious to cgroups and gives the processor count of the host, not what's available to the container.
If the answer is 'no', we still don't have a solution to the issue that was marked closed.
There was a problem hiding this comment.
I'm lousy at reading C code, but I think this is saying:
libuv/libuv@f250c6c
https://www.cygwin.com/bugzilla/show_bug.cgi?id=27645
That sysconf() works on musl (eg, Alpine Linux?) but not on glibc (eg, Ubuntu, CoreOS). But the extra call in the conditional block in the libuv commit above emulates the musl behavior. If I've followed that right, then the answer is 'yes' and it's not oblivious to cgroups.
There was a problem hiding this comment.
It's oblivious in the "cache oblivious algorithm" sense: libuv doesn't have to care whether cgroups are active or not; if the container is set up properly, it'll produce the right answer.
|
LGTM but I would like to see the doc comment addressed. |
addaleax
added
the
semver-minor
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
With the introduction of os.availableParallelism(), users should no longer rely on os.cpus().length to determine the amount of available parallelism. This commit adds a note to the os.cpus() docs. PR-URL: #45895 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Notable changes: buffer: * (SEMVER-MINOR) add buffer.isUtf8 for utf8 validation (Yagiz Nizipli) #45947 http: * (SEMVER-MINOR) improved timeout defaults handling (Paolo Insogna) #45778 net * add autoSelectFamily global getter and setter (Paolo Insogna) #45777 os: * (SEMVER-MINOR) add availableParallelism() (Colin Ihrig) #45895 util: * add fast path for text-decoder fatal flag (Yagiz Nizipli) #45803 PR-URL: #46061
Notable changes: buffer: * (SEMVER-MINOR) add buffer.isUtf8 for utf8 validation (Yagiz Nizipli) #45947 http: * (SEMVER-MINOR) improved timeout defaults handling (Paolo Insogna) #45778 net * add autoSelectFamily global getter and setter (Paolo Insogna) #45777 os: * (SEMVER-MINOR) add availableParallelism() (Colin Ihrig) #45895 util: * add fast path for text-decoder fatal flag (Yagiz Nizipli) #45803 PR-URL: #46061
Notable changes: buffer: * (SEMVER-MINOR) add buffer.isUtf8 for utf8 validation (Yagiz Nizipli) #45947 http: * (SEMVER-MINOR) improved timeout defaults handling (Paolo Insogna) #45778 net * add autoSelectFamily global getter and setter (Paolo Insogna) #45777 os: * (SEMVER-MINOR) add availableParallelism() (Colin Ihrig) #45895 util: * add fast path for text-decoder fatal flag (Yagiz Nizipli) #45803 PR-URL: #46061
Refs: nodejs#45895 PR-URL: nodejs#46003 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Refs: nodejs#45895 PR-URL: nodejs#46003 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
This commit exposes uv_available_parallelism() as an alternative to cpus().length. uv_available_parallelism() is inspired by Rust's available_parallelism(). PR-URL: #45895 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
With the introduction of os.availableParallelism(), users should no longer rely on os.cpus().length to determine the amount of available parallelism. This commit adds a note to the os.cpus() docs. PR-URL: #45895 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This commit exposes uv_available_parallelism() as an alternative to cpus().length. uv_available_parallelism() is inspired by Rust's available_parallelism(). PR-URL: #45895 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
With the introduction of os.availableParallelism(), users should no longer rely on os.cpus().length to determine the amount of available parallelism. This commit adds a note to the os.cpus() docs. PR-URL: #45895 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Notable changes: * deps: * upgrade npm to 9.3.1 (npm team) #46242 * doc: * add parallelism note to os.cpus() (Colin Ihrig) #45895 * http: * join authorization headers (Marco Ippolito) #45982 * improved timeout defaults handling (Paolo Insogna) #45778 * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205 PR-URL: #46396
Notable changes: * deps: * upgrade npm to 9.3.1 (npm team) #46242 * doc: * add parallelism note to os.cpus() (Colin Ihrig) #45895 * http: * join authorization headers (Marco Ippolito) #45982 * improved timeout defaults handling (Paolo Insogna) #45778 * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205 PR-URL: #46396
Notable changes: * deps: * upgrade npm to 9.3.1 (npm team) #46242 * doc: * add parallelism note to os.cpus() (Colin Ihrig) #45895 * http: * join authorization headers (Marco Ippolito) #45982 * improved timeout defaults handling (Paolo Insogna) #45778 * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205 PR-URL: #46396
Notable changes: * deps: * upgrade npm to 9.3.1 (npm team) #46242 * doc: * add parallelism note to os.cpus() (Colin Ihrig) #45895 * http: * join authorization headers (Marco Ippolito) #45982 * improved timeout defaults handling (Paolo Insogna) #45778 * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205 PR-URL: #46396
This commit exposes
uv_available_parallelism()as an alternative tocpus().length.uv_available_parallelism()is inspired by Rust'savailable_parallelism().